-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added to-do-app #21
base: master
Are you sure you want to change the base?
Added to-do-app #21
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Jai - good job on the assignment! 🚀 In general you seem to have a good grasp on React and how to work with components. I have a few things in general I think you could look at to become even better:
- Could you refactor your app so you don't have to use ".getElementById"? It is not really the React way of doing things.
- If the user inputs (todo text and deadline) is not valid, could the user get a message of some sort to know what they have done wrong?
- Your code formatting could be a little more strict. A few places you have some indented code that should not be indented, some missing spaces here and there etc.
Let me know if you have any questions about my feedback 👍 Have a nice weekend!
nowDate.setHours(0,0,0,0); | ||
date.setHours(0,0,0,0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to set these? new Date() should just return the current time.
const onAddToDo = () => { | ||
const newDescription=document.getElementById('newDescription').value; | ||
const newDeadLine=startDate.getFullYear()+'-'+(startDate.getMonth()+1)+'-'+startDate.getDate(); | ||
const validDesc=(newDescription.length>0)?true:false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const validDesc=(newDescription.length>0)?true:false; | |
const validDesc = newDescription.length > 0; |
In this case you don't need a ternary operator, you can just check if the condition is true or false like this.
{todos.length>0 | ||
? | ||
todos.map((item,index) => | ||
<ToDo key={index+1} id={index+1} description={item.description} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add the +1 in key and id?
useEffect(() => { | ||
let secCounter=0; | ||
setInterval(function(){ | ||
secCounter+=1; | ||
document.getElementById('counter').innerHTML='You have used '+ secCounter +' seconds'; | ||
}, 1000); | ||
return () => {}; | ||
}, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be set in it's own component, for example in a component called Timer or something?
return date.getTime()>=nowDate.getTime(); | ||
}; | ||
const onAddToDo = () => { | ||
const newDescription=document.getElementById('newDescription').value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general when using React it should not be necessary to use the "document.getElementById". In this case, it would be better to have a state variable that holds the text that is currently written in the input field. Then you could use that state instead of using "getElementById". Let me know if it makes sense, else I would be happy to show a more elaborate example.
setEditMode(true); | ||
} | ||
const handleUpdate = (id) => { | ||
const updatedDesc=document.getElementById('input'+id).value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of the same thing as I commented on above. It should not really be necessary to use getElementById, there are better ways to do it when we are working with React.
const loadData = async () => { | ||
const res = await fetch(DEFAULT_API_URL); | ||
setTodos(await res.json()); | ||
}; | ||
useEffect(() => { | ||
loadData(); | ||
return () => {}; | ||
}, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job of abstracting it into it's own function. If you wanted, you could put the loadData function in it's own file. That would create a better file structure once the app grows and has more api calls.
React/Week3 Homework